-
Notifications
You must be signed in to change notification settings - Fork 15.4k
[lldb] Properly cache the result of MachProcess::GetPlatform #140611
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
788597d to
7ccef56
Compare
|
@llvm/pr-subscribers-lldb Author: Tal Keren (talkeren) ChangesIf Full diff: https://github.com/llvm/llvm-project/pull/140611.diff 2 Files Affected:
diff --git a/lldb/tools/debugserver/source/MacOSX/MachProcess.h b/lldb/tools/debugserver/source/MacOSX/MachProcess.h
index 56bc9d6c7461e..516a77c0bd6c8 100644
--- a/lldb/tools/debugserver/source/MacOSX/MachProcess.h
+++ b/lldb/tools/debugserver/source/MacOSX/MachProcess.h
@@ -398,7 +398,7 @@ class MachProcess {
pid_t m_pid; // Process ID of child process
cpu_type_t m_cpu_type; // The CPU type of this process
- uint32_t m_platform; // The platform of this process
+ std::optional<uint32_t> m_platform; // The platform of this process
int m_child_stdin;
int m_child_stdout;
int m_child_stderr;
diff --git a/lldb/tools/debugserver/source/MacOSX/MachProcess.mm b/lldb/tools/debugserver/source/MacOSX/MachProcess.mm
index 3afaaa2f64c00..ce74dd1cadfb0 100644
--- a/lldb/tools/debugserver/source/MacOSX/MachProcess.mm
+++ b/lldb/tools/debugserver/source/MacOSX/MachProcess.mm
@@ -1039,9 +1039,9 @@ static bool mach_header_validity_test(uint32_t magic, uint32_t cputype) {
};
uint32_t MachProcess::GetPlatform() {
- if (m_platform == 0)
+ if (!m_platform)
m_platform = MachProcess::GetProcessPlatformViaDYLDSPI();
- return m_platform;
+ return *m_platform;
}
uint32_t MachProcess::GetProcessPlatformViaDYLDSPI() {
@@ -1058,7 +1058,9 @@ static bool mach_header_validity_test(uint32_t magic, uint32_t cputype) {
}
return platform;
}
-
+If `MachProcess::GetProcessPlatformViaDYLDSPI` fails and return 0, we don't
+want to call it everytime as it won't change the result. So use
+std::optional to cache wether we already calculated the value.
void MachProcess::GetAllLoadedBinariesViaDYLDSPI(
std::vector<struct binary_image_information> &image_infos) {
kern_return_t kern_ret;
@@ -1323,7 +1325,7 @@ static bool mach_header_validity_test(uint32_t magic, uint32_t cputype) {
// Clear any cached thread list while the pid and task are still valid
m_task.Clear();
- m_platform = 0;
+ m_platform.reset();
// Now clear out all member variables
m_pid = INVALID_NUB_PROCESS;
if (!detaching)
@@ -1754,7 +1756,7 @@ static uint64_t bits(uint64_t value, uint32_t msbit, uint32_t lsbit) {
// NULL our task out as we have already restored all exception ports
m_task.Clear();
- m_platform = 0;
+ m_platform.reset();
// Clear out any notion of the process we once were
const bool detaching = true;
|
7ccef56 to
c385e97
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
JDevlieghere
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I'd like @jasonmolenda to sign off as the debugserver maintainer.
|
Closing it following the discussion in the issue - I made a new PR that fixes it differently without change the current behaviour. |
If
MachProcess::GetProcessPlatformViaDYLDSPIfails and return 0, we don'twant to call it everytime as it won't change the result. So use
std::optional to cache wether we already calculated the value.
Alleviate the impact of issue #140610